[mlir][LLVMIR] Add IFuncOp to LLVM dialect #147697
Open
+499
−31
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add IFunc to LLVM dialect and add support for lifting/exporting LLVMIR IFunc.
|
@llvm/pr-subscribers-mlir-llvm @llvm/pr-subscribers-mlir Author: Robert Konicar (Jezurko) ChangesAdd IFunc to LLVM dialect and add support for lifting/exporting LLVMIR IFunc. Patch is 31.51 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/147697.diff 10 Files Affected:
diff --git a/mlir/include/mlir/Dialect/LLVMIR/LLVMDialect.h b/mlir/include/mlir/Dialect/LLVMIR/LLVMDialect.h
index 63e007cdc335c..e355bb8f5ddae 100644
--- a/mlir/include/mlir/Dialect/LLVMIR/LLVMDialect.h
+++ b/mlir/include/mlir/Dialect/LLVMIR/LLVMDialect.h
@@ -223,6 +223,9 @@ Value createGlobalString(Location loc, OpBuilder &builder, StringRef name,
/// function confirms that the Operation has the desired properties.
bool satisfiesLLVMModule(Operation *op);
+/// Lookup parent Module satisfying LLVM conditions on the Module Operation.
+Operation *parentLLVMModule(Operation *op);
+
/// Convert an array of integer attributes to a vector of integers that can be
/// used as indices in LLVM operations.
template <typename IntT = int64_t>
diff --git a/mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td b/mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td
index f4c1640098320..fe1418b12b90a 100644
--- a/mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td
+++ b/mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td
@@ -1285,6 +1285,10 @@ def LLVM_AddressOfOp : LLVM_Op<"mlir.addressof",
/// Return the llvm.mlir.alias operation that defined the value referenced
/// here.
AliasOp getAlias(SymbolTableCollection &symbolTable);
+
+ /// Return the llvm.mlir.ifunc operation that defined the value referenced
+ /// here.
+ IFuncOp getIFunc(SymbolTableCollection &symbolTable);
}];
let assemblyFormat = "$global_name attr-dict `:` qualified(type($res))";
@@ -1601,6 +1605,42 @@ def LLVM_AliasOp : LLVM_Op<"mlir.alias",
let hasRegionVerifier = 1;
}
+def LLVM_IFuncOp : LLVM_Op<"mlir.ifunc",
+ [IsolatedFromAbove, Symbol, DeclareOpInterfaceMethods<SymbolUserOpInterface>]> {
+ let arguments = (ins
+ SymbolNameAttr:$sym_name,
+ TypeAttr:$i_func_type,
+ FlatSymbolRefAttr:$resolver,
+ TypeAttr:$resolver_type,
+ UnitAttr:$dso_local,
+ DefaultValuedAttr<ConfinedAttr<I32Attr, [IntNonNegative]>, "0">:$address_space,
+ DefaultValuedAttr<Linkage, "mlir::LLVM::Linkage::External">:$linkage,
+ DefaultValuedAttr<UnnamedAddr, "mlir::LLVM::UnnamedAddr::None">:$unnamed_addr,
+ DefaultValuedAttr<Visibility, "mlir::LLVM::Visibility::Default">:$visibility_
+ );
+ let summary = "LLVM dialect ifunc";
+ let description = [{
+ `llvm.mlir.ifunc` is a top level operation that defines a global ifunc.
+ It defines a new symbol and takes a symbol refering to a resolver function.
+ IFuncs can be called as regular functions. The function type is the same
+ as the IFuncType. The symbol is resolved at runtime by calling a resolver
+ function.
+ }];
+
+ let builders = [
+ OpBuilder<(ins "StringRef":$name, "Type":$i_func_type,
+ "StringRef":$resolver, "Type":$resolver_type,
+ "Linkage":$linkage, "LLVM::Visibility":$visibility)>
+ ];
+
+ let assemblyFormat = [{
+ (custom<LLVMLinkage>($linkage)^)? ($visibility_^)? ($unnamed_addr^)?
+ $sym_name `:` $i_func_type `,` $resolver_type $resolver attr-dict
+ }];
+ let hasVerifier = 1;
+}
+
+
def LLVM_DSOLocalEquivalentOp : LLVM_Op<"dso_local_equivalent",
[Pure, ConstantLike, DeclareOpInterfaceMethods<SymbolUserOpInterface>]> {
let arguments = (ins FlatSymbolRefAttr:$function_name);
diff --git a/mlir/include/mlir/Target/LLVMIR/ModuleImport.h b/mlir/include/mlir/Target/LLVMIR/ModuleImport.h
index 9902c6bb15caf..b21600b634d2e 100644
--- a/mlir/include/mlir/Target/LLVMIR/ModuleImport.h
+++ b/mlir/include/mlir/Target/LLVMIR/ModuleImport.h
@@ -71,6 +71,9 @@ class ModuleImport {
/// Converts all aliases of the LLVM module to MLIR variables.
LogicalResult convertAliases();
+ /// Converts all ifuncs of the LLVM module to MLIR variables.
+ LogicalResult convertIFuncs();
+
/// Converts the data layout of the LLVM module to an MLIR data layout
/// specification.
LogicalResult convertDataLayout();
@@ -320,6 +323,8 @@ class ModuleImport {
/// Converts an LLVM global alias variable into an MLIR LLVM dialect alias
/// operation if a conversion exists. Otherwise, returns failure.
LogicalResult convertAlias(llvm::GlobalAlias *alias);
+ // Converts an LLVM global ifunc into an MLIR LLVM diaeclt ifunc operation
+ LogicalResult convertIFunc(llvm::GlobalIFunc *ifunc);
/// Returns personality of `func` as a FlatSymbolRefAttr.
FlatSymbolRefAttr getPersonalityAsAttr(llvm::Function *func);
/// Imports `bb` into `block`, which must be initially empty.
diff --git a/mlir/include/mlir/Target/LLVMIR/ModuleTranslation.h b/mlir/include/mlir/Target/LLVMIR/ModuleTranslation.h
index 79e8bb6add0da..fc82be3b88395 100644
--- a/mlir/include/mlir/Target/LLVMIR/ModuleTranslation.h
+++ b/mlir/include/mlir/Target/LLVMIR/ModuleTranslation.h
@@ -223,6 +223,12 @@ class ModuleTranslation {
return aliasesMapping.lookup(op);
}
+ /// Finds an LLVM IR global value that corresponds to the given MLIR operation
+ /// defining an IFunc.
+ llvm::GlobalValue *lookupIFunc(Operation *op) {
+ return ifuncMapping.lookup(op);
+ }
+
/// Returns the OpenMP IR builder associated with the LLVM IR module being
/// constructed.
llvm::OpenMPIRBuilder *getOpenMPBuilder();
@@ -308,6 +314,7 @@ class ModuleTranslation {
bool recordInsertions = false);
LogicalResult convertFunctionSignatures();
LogicalResult convertFunctions();
+ LogicalResult convertIFuncs();
LogicalResult convertComdats();
LogicalResult convertUnresolvedBlockAddress();
@@ -369,6 +376,8 @@ class ModuleTranslation {
/// aliases.
DenseMap<Operation *, llvm::GlobalValue *> aliasesMapping;
+ DenseMap<Operation *, llvm::GlobalValue *> ifuncMapping;
+
/// A stateful object used to translate types.
TypeToLLVMIRTranslator typeTranslator;
diff --git a/mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp b/mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp
index 6dcd94e6eea17..da5a3f40faaa3 100644
--- a/mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp
+++ b/mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp
@@ -139,6 +139,17 @@ static RetTy parseOptionalLLVMKeyword(OpAsmParser &parser,
return static_cast<RetTy>(index);
}
+static void printLLVMLinkage(OpAsmPrinter &p, Operation *, LinkageAttr val) {
+ p << stringifyLinkage(val.getLinkage());
+}
+
+static OptionalParseResult parseLLVMLinkage(OpAsmParser &p, LinkageAttr &val) {
+ val = LinkageAttr::get(
+ p.getContext(),
+ parseOptionalLLVMKeyword<LLVM::Linkage>(p, LLVM::Linkage::External));
+ return success();
+}
+
//===----------------------------------------------------------------------===//
// Operand bundle helpers.
//===----------------------------------------------------------------------===//
@@ -1175,14 +1186,17 @@ LogicalResult CallOp::verifySymbolUses(SymbolTableCollection &symbolTable) {
return emitOpError()
<< "'" << calleeName.getValue()
<< "' does not reference a symbol in the current scope";
- auto fn = dyn_cast<LLVMFuncOp>(callee);
- if (!fn)
- return emitOpError() << "'" << calleeName.getValue()
- << "' does not reference a valid LLVM function";
-
- if (failed(verifyCallOpDebugInfo(*this, fn)))
- return failure();
- fnType = fn.getFunctionType();
+ if (auto fn = dyn_cast<LLVMFuncOp>(callee)) {
+ if (failed(verifyCallOpDebugInfo(*this, fn)))
+ return failure();
+ fnType = fn.getFunctionType();
+ } else if (auto ifunc = dyn_cast<IFuncOp>(callee)) {
+ fnType = ifunc.getIFuncType();
+ } else {
+ return emitOpError()
+ << "'" << calleeName.getValue()
+ << "' does not reference a valid LLVM function or IFunc";
+ }
}
LLVMFunctionType funcType = llvm::dyn_cast<LLVMFunctionType>(fnType);
@@ -2038,14 +2052,6 @@ LogicalResult ReturnOp::verify() {
// LLVM::AddressOfOp.
//===----------------------------------------------------------------------===//
-static Operation *parentLLVMModule(Operation *op) {
- Operation *module = op->getParentOp();
- while (module && !satisfiesLLVMModule(module))
- module = module->getParentOp();
- assert(module && "unexpected operation outside of a module");
- return module;
-}
-
GlobalOp AddressOfOp::getGlobal(SymbolTableCollection &symbolTable) {
return dyn_cast_or_null<GlobalOp>(
symbolTable.lookupSymbolIn(parentLLVMModule(*this), getGlobalNameAttr()));
@@ -2061,6 +2067,11 @@ AliasOp AddressOfOp::getAlias(SymbolTableCollection &symbolTable) {
symbolTable.lookupSymbolIn(parentLLVMModule(*this), getGlobalNameAttr()));
}
+IFuncOp AddressOfOp::getIFunc(SymbolTableCollection &symbolTable) {
+ return dyn_cast_or_null<IFuncOp>(
+ symbolTable.lookupSymbolIn(parentLLVMModule(*this), getGlobalNameAttr()));
+}
+
LogicalResult
AddressOfOp::verifySymbolUses(SymbolTableCollection &symbolTable) {
Operation *symbol =
@@ -2069,10 +2080,11 @@ AddressOfOp::verifySymbolUses(SymbolTableCollection &symbolTable) {
auto global = dyn_cast_or_null<GlobalOp>(symbol);
auto function = dyn_cast_or_null<LLVMFuncOp>(symbol);
auto alias = dyn_cast_or_null<AliasOp>(symbol);
+ auto ifunc = dyn_cast_or_null<IFuncOp>(symbol);
- if (!global && !function && !alias)
+ if (!global && !function && !alias && !ifunc)
return emitOpError("must reference a global defined by 'llvm.mlir.global', "
- "'llvm.mlir.alias' or 'llvm.func'");
+ "'llvm.mlir.alias' or 'llvm.func' or 'llvm.mlir.ifunc'");
LLVMPointerType type = getType();
if ((global && global.getAddrSpace() != type.getAddressSpace()) ||
@@ -2682,6 +2694,56 @@ unsigned AliasOp::getAddrSpace() {
return ptrTy.getAddressSpace();
}
+//===----------------------------------------------------------------------===//
+// IFuncOp
+//===----------------------------------------------------------------------===//
+
+void IFuncOp::build(OpBuilder &builder, OperationState &result, StringRef name,
+ Type iFuncType, StringRef resolverName, Type resolverType,
+ Linkage linkage, LLVM::Visibility visibility) {
+ return build(builder, result, name, iFuncType, resolverName, resolverType,
+ /* dso_local */ false, /* addr_space */ 0, linkage,
+ UnnamedAddr::None, visibility);
+}
+LogicalResult IFuncOp::verifySymbolUses(SymbolTableCollection &symbolTable) {
+ return success();
+ Operation *symbol =
+ symbolTable.lookupSymbolIn(parentLLVMModule(*this), getResolverAttr());
+ auto resolver = dyn_cast<LLVMFuncOp>(symbol);
+ if (!resolver)
+ return emitOpError("IFunc must have a Function resolver");
+
+ // Copying logic from llvm/lib/IR/Verifier.cpp
+ Linkage linkage = resolver.getLinkage();
+ if (resolver.isExternal() || linkage == Linkage::AvailableExternally)
+ return emitOpError("IFunc resolver must be a definition");
+ if (!isa<LLVMPointerType>(resolver.getFunctionType().getReturnType()))
+ return emitOpError("IFunc resolver must return a pointer");
+ auto resolverPtr = dyn_cast<LLVMPointerType>(getResolverType());
+ if (!resolverPtr || resolverPtr.getAddressSpace() != getAddressSpace())
+ return emitOpError("IFunc resolver has incorrect type");
+ return success();
+}
+
+LogicalResult IFuncOp::verify() {
+ switch (getLinkage()) {
+ case Linkage::External:
+ case Linkage::Internal:
+ case Linkage::Private:
+ case Linkage::Weak:
+ case Linkage::WeakODR:
+ case Linkage::Linkonce:
+ case Linkage::LinkonceODR:
+ break;
+ default:
+ return emitOpError() << "'" << stringifyLinkage(getLinkage())
+ << "' linkage not supported in ifuncs, available "
+ "options: private, internal, linkonce, weak, "
+ "linkonce_odr, weak_odr, or external linkage";
+ }
+ return success();
+}
+
//===----------------------------------------------------------------------===//
// ShuffleVectorOp
//===----------------------------------------------------------------------===//
@@ -4329,3 +4391,11 @@ bool mlir::LLVM::satisfiesLLVMModule(Operation *op) {
return op->hasTrait<OpTrait::SymbolTable>() &&
op->hasTrait<OpTrait::IsIsolatedFromAbove>();
}
+
+Operation *mlir::LLVM::parentLLVMModule(Operation *op) {
+ Operation *module = op->getParentOp();
+ while (module && !satisfiesLLVMModule(module))
+ module = module->getParentOp();
+ assert(module && "unexpected operation outside of a module");
+ return module;
+}
diff --git a/mlir/lib/Target/LLVMIR/Dialect/LLVMIR/LLVMToLLVMIRTranslation.cpp b/mlir/lib/Target/LLVMIR/Dialect/LLVMIR/LLVMToLLVMIRTranslation.cpp
index 70029d7e15a90..9a648ecd3a8d2 100644
--- a/mlir/lib/Target/LLVMIR/Dialect/LLVMIR/LLVMToLLVMIRTranslation.cpp
+++ b/mlir/lib/Target/LLVMIR/Dialect/LLVMIR/LLVMToLLVMIRTranslation.cpp
@@ -422,9 +422,18 @@ convertOperationImpl(Operation &opInst, llvm::IRBuilderBase &builder,
ArrayRef<llvm::Value *> operandsRef(operands);
llvm::CallInst *call;
if (auto attr = callOp.getCalleeAttr()) {
- call =
- builder.CreateCall(moduleTranslation.lookupFunction(attr.getValue()),
- operandsRef, opBundles);
+ if (llvm::Function *function =
+ moduleTranslation.lookupFunction(attr.getValue())) {
+ call = builder.CreateCall(function, operandsRef, opBundles);
+ } else {
+ Operation *module = parentLLVMModule(&opInst);
+ Operation *ifuncOp =
+ moduleTranslation.symbolTable().lookupSymbolIn(module, attr);
+ llvm::GlobalValue *ifunc = moduleTranslation.lookupIFunc(ifuncOp);
+ llvm::FunctionType *calleeType = llvm::cast<llvm::FunctionType>(
+ moduleTranslation.convertType(callOp.getCalleeFunctionType()));
+ call = builder.CreateCall(calleeType, ifunc, operandsRef, opBundles);
+ }
} else {
llvm::FunctionType *calleeType = llvm::cast<llvm::FunctionType>(
moduleTranslation.convertType(callOp.getCalleeFunctionType()));
@@ -648,18 +657,21 @@ convertOperationImpl(Operation &opInst, llvm::IRBuilderBase &builder,
LLVM::LLVMFuncOp function =
addressOfOp.getFunction(moduleTranslation.symbolTable());
LLVM::AliasOp alias = addressOfOp.getAlias(moduleTranslation.symbolTable());
+ LLVM::IFuncOp ifunc = addressOfOp.getIFunc(moduleTranslation.symbolTable());
// The verifier should not have allowed this.
- assert((global || function || alias) &&
- "referencing an undefined global, function, or alias");
+ assert((global || function || alias || ifunc) &&
+ "referencing an undefined global, function, alias, or ifunc");
llvm::Value *llvmValue = nullptr;
if (global)
llvmValue = moduleTranslation.lookupGlobal(global);
else if (alias)
llvmValue = moduleTranslation.lookupAlias(alias);
- else
+ else if (function)
llvmValue = moduleTranslation.lookupFunction(function.getName());
+ else
+ llvmValue = moduleTranslation.lookupIFunc(ifunc);
moduleTranslation.mapValue(addressOfOp.getResult(), llvmValue);
return success();
diff --git a/mlir/lib/Target/LLVMIR/ModuleImport.cpp b/mlir/lib/Target/LLVMIR/ModuleImport.cpp
index bfda223fe0f5f..a88e1c9847fcf 100644
--- a/mlir/lib/Target/LLVMIR/ModuleImport.cpp
+++ b/mlir/lib/Target/LLVMIR/ModuleImport.cpp
@@ -1031,6 +1031,16 @@ LogicalResult ModuleImport::convertAliases() {
return success();
}
+LogicalResult ModuleImport::convertIFuncs() {
+ for (llvm::GlobalIFunc &ifunc : llvmModule->ifuncs()) {
+ if (failed(convertIFunc(&ifunc))) {
+ return emitError(UnknownLoc::get(context))
+ << "unhandled global ifunc: " << diag(ifunc);
+ }
+ }
+ return success();
+}
+
LogicalResult ModuleImport::convertDataLayout() {
Location loc = mlirModule.getLoc();
DataLayoutImporter dataLayoutImporter(context, llvmModule->getDataLayout());
@@ -1369,6 +1379,21 @@ LogicalResult ModuleImport::convertAlias(llvm::GlobalAlias *alias) {
return success();
}
+LogicalResult ModuleImport::convertIFunc(llvm::GlobalIFunc *ifunc) {
+ OpBuilder::InsertionGuard guard = setGlobalInsertionPoint();
+
+ Type type = convertType(ifunc->getValueType());
+ llvm::Constant *resolver = ifunc->getResolver();
+ Type resolverType = convertType(resolver->getType());
+ builder.create<IFuncOp>(mlirModule.getLoc(), ifunc->getName(), type,
+ resolver->getName(), resolverType,
+ ifunc->isDSOLocal(), ifunc->getAddressSpace(),
+ convertLinkageFromLLVM(ifunc->getLinkage()),
+ convertUnnamedAddrFromLLVM(ifunc->getUnnamedAddr()),
+ convertVisibilityFromLLVM(ifunc->getVisibility()));
+ return success();
+}
+
LogicalResult ModuleImport::convertGlobal(llvm::GlobalVariable *globalVar) {
// Insert the global after the last one or at the start of the module.
OpBuilder::InsertionGuard guard = setGlobalInsertionPoint();
@@ -1973,8 +1998,9 @@ ModuleImport::convertCallOperands(llvm::CallBase *callInst,
// treated as indirect calls to constant operands that need to be converted.
// Skip the callee operand if it's inline assembly, as it's handled separately
// in InlineAsmOp.
- if (!isa<llvm::Function>(callInst->getCalledOperand()) && !isInlineAsm) {
- FailureOr<Value> called = convertValue(callInst->getCalledOperand());
+ llvm::Value *caleeOperand = callInst->getCalledOperand();
+ if (!isa<llvm::Function, llvm::GlobalIFunc>(caleeOperand) && !isInlineAsm) {
+ FailureOr<Value> called = convertValue(caleeOperand);
if (failed(called))
return failure();
operands.push_back(*called);
@@ -2035,12 +2061,21 @@ ModuleImport::convertFunctionType(llvm::CallBase *callInst,
if (failed(callType))
return failure();
auto *callee = dyn_cast<llvm::Function>(calledOperand);
+
+ llvm::FunctionType *origCalleeType = nullptr;
+ if (callee) {
+ origCalleeType = callee->getFunctionType();
+ } else if (auto *ifunc = dyn_cast<llvm::GlobalIFunc>(calledOperand)) {
+ origCalleeType =
+ dyn_cast_or_null<llvm::FunctionType>(ifunc->getValueType());
+ }
+
// For indirect calls, return the type of the call itself.
- if (!callee)
+ if (!origCalleeType)
return callType;
FailureOr<LLVMFunctionType> calleeType =
- castOrFailure(convertType(callee->getFunctionType()));
+ castOrFailure(convertType(origCalleeType));
if (failed(calleeType))
return failure();
@@ -2059,8 +2094,8 @@ ModuleImport::convertFunctionType(llvm::CallBase *callInst,
FlatSymbolRefAttr ModuleImport::convertCalleeName(llvm::CallBase *callInst) {
llvm::Value *calledOperand = callInst->getCalledOperand();
- if (auto *callee = dyn_cast<llvm::Function>(calledOperand))
- return SymbolRefAttr::get(context, callee->getName());
+ if (isa<llvm::Function, llvm::GlobalIFunc>(calledOperand))
+ return SymbolRefAttr::get(context, calledOperand->getName());
return {};
}
@@ -3162,6 +3197,8 @@ OwningOpRef<ModuleOp> mlir::translateLLVMIRToModule(
return {};
if (failed(moduleImport.convertAliases()))
return {};
+ if (failed(moduleImport.convertIFuncs()))
+ return {};
moduleImport.convertTargetTriple();
return module;
}
diff --git a/mlir/lib/Target/LLVMIR/ModuleTranslation.cpp b/mlir/lib/Target/LLVMIR/ModuleTranslation.cpp
index 8908703cc1368..1a2a585e34d44 100644
--- a/mlir/lib/Target/LLVMIR/ModuleTranslation.cpp
+++ b/mlir/lib/Target/LLVMIR/ModuleTranslation.cpp
@@ -791,6 +791,8 @@ void ModuleTranslation::forgetMapping(Region ®ion) {
globalsMapping.erase(&op);
if (isa<LLVM::AliasOp>(op))
aliasesMapping.erase(&op);
+ if (isa<LLVM::IFuncOp>(op))
+ ifuncMapping.erase(&op);
if (isa<LLVM::CallOp>(op))
callMapping.erase(&op);
llvm::append_range(
@@ -1868,6 +1870,27 @@ LogicalResult ModuleTranslation::convertFunctions() {
return success();
}
+LogicalResult ModuleTranslation::convertIFuncs() {
+ for (auto op : getModuleBody(mlirModule).getOps<IFuncOp>()) {
+ llvm::Type *type = convertType(op.getIFuncType());
+ llvm::GlobalValue::LinkageTypes linkage =
+ convertLinkageToLLVM(op.getLinkage());
+ llvm::Constant *cst =
+ dyn_cast<llvm::Constant>(lookupFunction(op.getResolver()));
+
+ auto *ifunc =
+ llvm::GlobalIFunc::create(type, op.getAddressSpace(), linkage,
+ op.getSymName(), cst, llvmMod...
[truncated]
|
gysit
reviewed
Jul 9, 2025
There was a problem hiding this comment.
Nice! Thanks for adding ifuncs.
The direction looks good module some nits. The tests need a bit more work though. The current ones are a bit long and could probably be simplified? Additionally, adding some tests for the verifiers (invalid.mlir) and possibly some roundtrip tests for the custom printing and parsing would make sense.
| Type iFuncType, StringRef resolverName, Type resolverType, | ||
| Linkage linkage, LLVM::Visibility visibility) { | ||
| return build(builder, result, name, iFuncType, resolverName, resolverType, | ||
| /* dso_local */ false, /* addr_space */ 0, linkage, |
There was a problem hiding this comment.
Suggested change
| /* dso_local */ false, /* addr_space */ 0, linkage, | |
| /*dso_local=*/false, /*addr_space=*/0, linkage, |
nit: style
| UnnamedAddr::None, visibility); | ||
| } | ||
| LogicalResult IFuncOp::verifySymbolUses(SymbolTableCollection &symbolTable) { | ||
| return success(); |
There was a problem hiding this comment.
That makes me believe the PR needs some more verifier tests!
Comment on lines
2707
to
2708
| } | ||
| LogicalResult IFuncOp::verifySymbolUses(SymbolTableCollection &symbolTable) { |
There was a problem hiding this comment.
Suggested change
| } | |
| LogicalResult IFuncOp::verifySymbolUses(SymbolTableCollection &symbolTable) { | |
| } | |
| LogicalResult IFuncOp::verifySymbolUses(SymbolTableCollection &symbolTable) { |
nit missing newline
| llvm::Type *type = convertType(op.getIFuncType()); | ||
| llvm::GlobalValue::LinkageTypes linkage = | ||
| convertLinkageToLLVM(op.getLinkage()); | ||
| llvm::Constant *cst = |
There was a problem hiding this comment.
Suggested change
| llvm::Constant *cst = | |
| llvm::Constant *resolver = |
nit: I would probably go with resolver or similar to be more specific.
| moduleTranslation.lookupFunction(attr.getValue())) { | ||
| call = builder.CreateCall(function, operandsRef, opBundles); | ||
| } else { | ||
| Operation *module = parentLLVMModule(&opInst); |
There was a problem hiding this comment.
Suggested change
| Operation *module = parentLLVMModule(&opInst); | |
| Operation *moduleOp = parentLLVMModule(&opInst); |
nit: Let's use module op to avoid the red color.
I wonder if it would make sense if LLVM::CallOp would have a getFunction / getIFunc helper similar to the addressOf operation. But I suspect this would likely be the only use here?
There was a problem hiding this comment.
I don't think even the AddressOfOp versions are used anywhere else, so based on that precedent it would make sense to add it to the CallOp.
I also just noticed that dsoLocalEquivalentOp also has similar helpers, so adding it for consistency makes sense.
Not 100 % sure if I should introduce the change in this PR though?
There was a problem hiding this comment.
I do not have super strong opinion here.
I commented bcs parentLLVMModule moved to a header. I guess the helpers could be an alternative to exposing this helper function. But yeah I leave it up to you.
| if (!resolver) | ||
| return emitOpError("IFunc must have a Function resolver"); | ||
|
|
||
| // Copying logic from llvm/lib/IR/Verifier.cpp |
There was a problem hiding this comment.
Suggested change
| // Copying logic from llvm/lib/IR/Verifier.cpp | |
| // This matches LLVM IR verification logic, see from llvm/lib/IR/Verifier.cpp |
ultra nit: I suggest to avoid the term copying since it reminds of copy pasta :)
| @@ -1973,8 +1998,9 @@ ModuleImport::convertCallOperands(llvm::CallBase *callInst, | |||
| // treated as indirect calls to constant operands that need to be converted. | |||
| // Skip the callee operand if it's inline assembly, as it's handled separately | |||
| // in InlineAsmOp. | |||
| if (!isa<llvm::Function>(callInst->getCalledOperand()) && !isInlineAsm) { | |||
| FailureOr<Value> called = convertValue(callInst->getCalledOperand()); | |||
| llvm::Value *caleeOperand = callInst->getCalledOperand(); | |||
There was a problem hiding this comment.
Suggested change
| llvm::Value *caleeOperand = callInst->getCalledOperand(); | |
| llvm::Value *calleeOperand = callInst->getCalledOperand(); |
| origCalleeType = callee->getFunctionType(); | ||
| } else if (auto *ifunc = dyn_cast<llvm::GlobalIFunc>(calledOperand)) { | ||
| origCalleeType = | ||
| dyn_cast_or_null<llvm::FunctionType>(ifunc->getValueType()); |
There was a problem hiding this comment.
Shouldn't getValueType always return a function type? Could we use cast here? Or dyn_cast?
| @@ -0,0 +1,115 @@ | |||
| ; RUN: mlir-translate --import-llvm %s -split-input-file | FileCheck %s | |||
|
|
|||
There was a problem hiding this comment.
The tests are quite long compared to what we actually check. Is it possible to significantly simplify them by dropping code that is actually not needed e.g. most of the function bodies?
Also can we check some more attributes such as linkage etc. Maybe use a minimal ifunc in some case and add all the optional attributes on a second ifunc?
mlir/test/Target/LLVMIR/ifunc.mlir
Outdated
| @@ -0,0 +1,123 @@ | |||
| // RUN: mlir-translate -mlir-to-llvmir %s -split-input-file | FileCheck %s | |||
|
|
|||
| llvm.mlir.global private unnamed_addr constant @__const.main.data(dense<[1, 2, 3, 4, 5, 6, 7, 8, 9, 10]> : tensor<10xi32>) {addr_space = 0 : i32, alignment = 16 : i64, dso_local} : !llvm.array<10 x i32> | |||
There was a problem hiding this comment.
Can you shorten this test as well as proposed for the import test?
Jezurko
commented
Jul 11, 2025
Comment on lines
+2714
to
+2716
| // FIXME: Strip aliases to find the called function | ||
| if (isa<AliasOp>(symbol)) | ||
| return success(); |
There was a problem hiding this comment.
I am not sure how to nicely/cleanly mirror the LLVM IR logic here - the LLVM verifier internally calls stripPointerCastsAndAliases to find the resolver function.
In the AliasOp init region there might be quite a lot of stuff happening, so backtracking through might be quite complicated… One idea I had was to look simply for AddressOps and jump based on those… But I'm not sure if that's sound.
There was a problem hiding this comment.
Yeah just lookin for AddressOfOp sounds dangerous that could in theory lead to false verification errors on correct code.
I would either just handle the trivial cases where the AliasOp returns a value that is directly defined by an AddressOfOp or we bail out in this case and just return success (which is what you implemented). Did you see real-world examples where there was a lot of code in an AddressOfOp?
Btw can an IFunc refer to another IFunc as well (I would hope not but one never knows)?
|
Thanks a lot for the review! I tried to apply the requested changes and reworked the tests. One new issue on which I'm not sure how to proceed is in the symbol usage verifier… I've added a review comment to the corresponding code with explanation of the problem |
gysit
reviewed
Jul 11, 2025
| @@ -320,6 +323,8 @@ class ModuleImport { | |||
| /// Converts an LLVM global alias variable into an MLIR LLVM dialect alias | |||
| /// operation if a conversion exists. Otherwise, returns failure. | |||
| LogicalResult convertAlias(llvm::GlobalAlias *alias); | |||
| // Converts an LLVM global ifunc into an MLIR LLVM dialect ifunc operation | |||
There was a problem hiding this comment.
Suggested change
| // Converts an LLVM global ifunc into an MLIR LLVM dialect ifunc operation | |
| // Converts an LLVM global ifunc into an MLIR LLVM dialect ifunc operation. |
nit: missing dot
| Examples: | ||
|
|
||
| ```mlir | ||
| // IFuncs have @-identifier and use a resolver function. |
There was a problem hiding this comment.
Suggested change
| // IFuncs have @-identifier and use a resolver function. | |
| // IFuncs resolve a symbol at runtime using a resovler function. |
nit: I would make it a bit more specific.
| return success(); | ||
| return emitOpError("must have a function resolver"); | ||
| } | ||
| // This matches LLVM IR verification logic, see from llvm/lib/IR/Verifier.cpp |
There was a problem hiding this comment.
Suggested change
| // This matches LLVM IR verification logic, see from llvm/lib/IR/Verifier.cpp | |
| // This matches LLVM IR verification logic, see llvm/lib/IR/Verifier.cpp |
nit: sorry that was probably my fish.
Comment on lines
+2714
to
+2716
| // FIXME: Strip aliases to find the called function | ||
| if (isa<AliasOp>(symbol)) | ||
| return success(); |
There was a problem hiding this comment.
Yeah just lookin for AddressOfOp sounds dangerous that could in theory lead to false verification errors on correct code.
I would either just handle the trivial cases where the AliasOp returns a value that is directly defined by an AddressOfOp or we bail out in this case and just return success (which is what you implemented). Did you see real-world examples where there was a lot of code in an AddressOfOp?
Btw can an IFunc refer to another IFunc as well (I would hope not but one never knows)?
| if (auto *resolverFn = lookupFunction(op.getResolver())) { | ||
| resolver = dyn_cast<llvm::Constant>(resolverFn); | ||
| } else { | ||
| Operation *aliasTrg = symbolTable().lookupSymbolIn(parentLLVMModule(op), |
There was a problem hiding this comment.
Suggested change
| Operation *aliasTrg = symbolTable().lookupSymbolIn(parentLLVMModule(op), | |
| Operation *aliasOp = symbolTable().lookupSymbolIn(parentLLVMModule(op), |
nit: not sure what Trg is?
| // ----- | ||
|
|
||
| // CHECK: @ifunc = linkonce_odr hidden ifunc | ||
|
|
There was a problem hiding this comment.
Suggested change
nit: I would format the same as above and skip the empty line (also in the tests below?)
| llvm.call @foo(%3, %4) : (!llvm.ptr {llvm.noundef}, i32 {llvm.noundef}) -> () | ||
| llvm.return | ||
| } | ||
| llvm.func @call_indirect_foo(%arg0: !llvm.ptr {llvm.noundef}, %arg1: i32 {llvm.noundef}) attributes {dso_local} { |
There was a problem hiding this comment.
Similar to the test above I would just pass all three arguments for the call to the function then no extra logic is needed and the test is easier to read an maintain.
Comment on lines
+6
to
+16
| define dso_local void @call_foo(ptr noundef %0, i32 noundef %1) { | ||
| %3 = alloca ptr, align 8 | ||
| %4 = alloca i32, align 4 | ||
| store ptr %0, ptr %3, align 8 | ||
| store i32 %1, ptr %4, align 4 | ||
| %5 = load ptr, ptr %3, align 8 | ||
| %6 = load i32, ptr %4, align 4 | ||
| ; CHECK: llvm.call @foo | ||
| call void @foo(ptr noundef %5, i32 noundef %6) | ||
| ret void | ||
| } |
There was a problem hiding this comment.
Suggested change
| define dso_local void @call_foo(ptr noundef %0, i32 noundef %1) { | |
| %3 = alloca ptr, align 8 | |
| %4 = alloca i32, align 4 | |
| store ptr %0, ptr %3, align 8 | |
| store i32 %1, ptr %4, align 4 | |
| %5 = load ptr, ptr %3, align 8 | |
| %6 = load i32, ptr %4, align 4 | |
| ; CHECK: llvm.call @foo | |
| call void @foo(ptr noundef %5, i32 noundef %6) | |
| ret void | |
| } | |
| define dso_local void @call_foo(ptr noundef %0, i32 noundef %1) { | |
| ; CHECK: llvm.call @foo | |
| call void @foo(ptr noundef %0, i32 noundef %1) | |
| ret void | |
| } |
nit: I would shorten this one.
| %6 = load ptr, ptr %5, align 8 | ||
| %7 = load ptr, ptr %3, align 8 | ||
| %8 = load i32, ptr %4, align 4 | ||
| call void %6(ptr noundef %7, i32 noundef %8) |
There was a problem hiding this comment.
Can we pass all three arguments as function arguments as well to shorten the test?
| convertLinkageToLLVM(op.getLinkage()); | ||
| llvm::Constant *resolver; | ||
| if (auto *resolverFn = lookupFunction(op.getResolver())) { | ||
| resolver = dyn_cast<llvm::Constant>(resolverFn); |
There was a problem hiding this comment.
Suggested change
| resolver = dyn_cast<llvm::Constant>(resolverFn); | |
| resolver = cast<llvm::Constant>(resolverFn); |
nit: I assume this could be a cast.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add IFunc to LLVM dialect and add support for lifting/exporting LLVMIR IFunc.